Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update mesh AABB when software skinning is used #53144

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

pouleyKetchoupp
Copy link
Contributor

Not updating the mesh properly can cause rendering issues in some cases, like shadows not updating properly.

This is 3.x only since software skinning is not implemented on master for now.

This change seems to have a limited impact on performance. In release_debug, I've only noticed a drop of around 4% when updating +100 models at once (mostly due to calculating the AABB itself, not dynamic shadows).

Fixes #53018

Not updating the mesh properly can cause rendering issues in some cases,
like shadows not updating properly.
@akien-mga akien-mga merged commit d4d4603 into godotengine:3.x Sep 28, 2021
@akien-mga
Copy link
Member

Thanks!

@lawnjelly
Copy link
Member

Just to note, it's usually better to update AABB in a high performance setting something like this:

The Godot AABB structure is not good for fast building, for this working with min max works better, than convert to position + size in a final step.

Also for skinning, often the approach used is to use a conservative oversize large bound, and not recalculate it. So being able to set a custom AABB, and if this is set, to not do the AABB calcs might be good.

Still this works to be going on with.

@pouleyKetchoupp pouleyKetchoupp deleted the software-skinning-aabb branch September 28, 2021 15:35
@pouleyKetchoupp
Copy link
Contributor Author

The Godot AABB structure is not good for fast building, for this working with min max works better, than convert to position + size in a final step.

It's amazing, I just made a test based on an equivalent of the portal code, and it works very well! This gets rid of most of the drop in performance, I'm going to open another PR.

Also for skinning, often the approach used is to use a conservative oversize large bound, and not recalculate it. So being able to set a custom AABB, and if this is set, to not do the AABB calcs might be good.

Makes sense too, although in this case, even if the AABB wasn't modified we would have to call an instance update because of displaced vertices, so from what I see it doesn't look like it would make a lot of difference. Might be useful in specific cases though, it's good to keep in mind there's more room for improvements.

@lawnjelly
Copy link
Member

lawnjelly commented Sep 29, 2021

It's amazing, I just made a test based on an equivalent of the portal code, and it works very well! This gets rid of most of the drop in performance, I'm going to open another PR.

Yeah it will reduce to just a couple of SIMD instructions per vertex I expect, whereas the default Godot mechanism for expanding will have all kinds of branching and extra work that only needs to be done once off.

This is a bit of a trade off and why some engines store AABBs / RECTs as min / maxes rather than position + size. The windows RECT structure for instance is left, top, right, bottom, presumably for these kinds of reasons. Storing as min / max is faster for building and intersection testing, storing as position + size is faster for moving.

The other great trick BTW (although can't be used here I don't think) is to store the max values of the AABB as negative max (or min as negative min). This means you can do intersection tests with all comparisons of the same sign, which means they can be combined easier in SIMD instructions. I do this in the BVH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when enable force software skinning, the shadow will become static
4 participants